Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added init with options #32

Merged
merged 4 commits into from
Jun 9, 2020
Merged

Conversation

pablogs9
Copy link
Member

@pablogs9 pablogs9 commented Jun 3, 2020

This PR adds a convenience function for init with options: rclc_support_init_with_options.

This init function is required when default options from rcl_get_zero_initialized_init_options and rcl_init_options_init are not enough. For example when configuring underlying RMW in micro-ROS.

Please let me know your opinions about this feature.

Copy link
Collaborator

@JanStaschulat JanStaschulat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just update the unit tests.

rc = rclc_support_fini(&support);
EXPECT_EQ(RCL_RET_OK, rc);
// test invalid arguments
rc = rclc_support_init(nullptr, 0, nullptr, &allocator);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pablogs9 use also rclc_support_init_with_options function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

rc = rclc_support_init(nullptr, 0, nullptr, &allocator);
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rc);
rcutils_reset_error();
rc = rclc_support_init(&support, 0, nullptr, nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pablogs9 use also rclc_support_init_with_options function as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

JanStaschulat
JanStaschulat previously approved these changes Jun 3, 2020
This was referenced Jun 5, 2020
Copy link

@julionce julionce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I review it in #33.

@pablogs9
Copy link
Member Author

pablogs9 commented Jun 8, 2020

Changes applied

@pablogs9 pablogs9 requested a review from julionce June 8, 2020 06:10
@pablogs9 pablogs9 merged commit ad11729 into master Jun 9, 2020
@pablogs9 pablogs9 deleted the feature/added_init_with_options branch June 9, 2020 05:08
norro pushed a commit that referenced this pull request Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants